-
Notifications
You must be signed in to change notification settings - Fork 765
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Updates to ethereumjs-config v2.0.0 (old) #886
Conversation
2809f87
to
939cea0
Compare
Seems to be pretty "en vogue" in the team these days to have no PR below 40 files changed. 😂 🤣 😅 |
haha oh yeah, careful with regenerating those docs too 😄 |
I can move the docs to another PR, I think that's the most sensible thing to do. |
@evertonfraga I don't care too much, think we will also get this reviewed with the docs, whatever is more convenient for you. |
099e073
to
3875a61
Compare
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
8ab611f
to
7108daf
Compare
After review, I'll release the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing to hold off the merge, looks good, thanks Everton for the great work on this!
packages/account/.prettierrc
Outdated
{ | ||
"semi": false, | ||
"singleQuote": true | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No merge blocker, but just for understanding: why are these settings added separately and not as some general configuration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@holgerd77 So the linting process can occur independently on the CI and locally, for each package.
I understand the repetition, and I can look into alternatives, but I'd rather play on the safer side first, where we can have more granular control over the ethereumjs-config
packages
"compilerOptions": { | ||
"outDir": "./dist.browser" | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These new browser builds needs also be anchored in package.json
I guess - see README from config PR, can be tackled in a separate PR though as well eventually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: have added this to the V5 Todo list, so this won't be forgotten.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would pass on this whole task to @cgewecke - so to eventually integrate and to check that these two new build targets are working as expected and eventually give side effects another thought - if both of you are ok with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@holgerd77 I am OK with it, and can follow up with @cgewecke on that.
'no-unused-vars': 'off', | ||
'prefer-const': 'off', | ||
'sonarjs/no-duplicated-branches': 'off', | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, so many exceptions for the VM necessary? 😄
No problem of course on this round. Nevertheless interesting to state and see.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes! there were so many eslint complaints. I decided to add those as exceptions for now, and ship the config sooner, as it's working perfectly.
We can tackle those in other PRs.
packages/vm/lib/evm/eei.ts
Outdated
this.refundGas(new BN(this._common.param('gasPrices', 'selfdestructRefund'))) | ||
this.refundGas( | ||
new BN(this._common.param('gasPrices', 'selfdestructRefund')) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually liked the printWidth
setting being extended to 100 lines (from 800 or so) on the old format package, this gives a bit more space and there was a somewhat longer discussion around this during the introduction of config until we decided upon this.
This seems to not have made it, was this intentional? (at least I am assuming during re-formattings like the above, haven't concretely checked).
Not sure, should we keep / post-introduce this, any opinions @cgewecke @ryanio or @jochem-brouwer ?
@evertonfraga in doubt and if too much process interrupting merge here anyhow, we'll get this settled out later, eventually with some follow-up PR after discussion just with the linting updates.
"@ethereumjs/account": "^3.0.0", | ||
"@ethereumjs/block": "^3.0.0", | ||
"@ethereumjs/blockchain": "^4.0.3", | ||
"@ethereumjs/common": "^1.5.1", | ||
"@ethereumjs/tx": "^2.1.2", | ||
"async-eventemitter": "^0.2.4", | ||
"core-js-pure": "^3.0.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these two new dependencies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@holgerd77 they were already there. I have just moved them to the correct order (@, A-Z)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reapproval after new commits.
@evertonfraga Thanks for the latest updates here, looks great 🙂 , let me know once you need a review! |
Superseded by #913, will close. |
This PR removes references to the old
ethereumjs-config
packages, replacing by the new ones. Configures all the new tooling.This PR uses gitpkg urls to test monorepo package dependencies prior to their release.
Remaining checks:
Fix lint results
Fix test runners
Other tasks